Skip to content

fix: keep shared HTTP/2 connections pool-owned - #937

Open
smartinio wants to merge 2 commits into
benoitc:masterfrom
smartinio:smartinio/fix-http2-pooled-sync-requests
Open

fix: keep shared HTTP/2 connections pool-owned#937
smartinio wants to merge 2 commits into
benoitc:masterfrom
smartinio:smartinio/fix-http2-pooled-sync-requests

Conversation

@smartinio

@smartinio smartinio commented Sep 4, 2026

Copy link
Copy Markdown

After upgrading Hackney from v1 to v4, we immediately started seeing concurrent production requests fail with {error, closed} while another request on the same connection completed successfully.

We traced this to v4 negotiating HTTP/2 by default and the pooled connection remaining owned by the process that created it. When a synchronous requester exits, the shared connection exits with it and interrupts other requests using its streams.

This PR transfers ownership to the pool before publishing a connection for reuse. Registration keeps an existing ready connection, replaces an unusable one, and lets a busy one drain before it stops.

Connection probes, ownership transfers, and candidate shutdowns are bounded. Registration carries a deadline derived from checkout_timeout, falling back to connect_timeout, so expired registrations are rejected even if they reach the pool later. Failed, expired, and duplicate candidates are stopped rather than exposed with uncertain ownership, allowing their per-host slots to be released.

A request waiting for a per-host slot now rechecks whether a busy HTTP/2 connection has become reusable without exceeding the existing checkout timeout. Each HTTP/2 stream also tracks its requester, so an abandoned streaming upload or response in {async, once} mode is cancelled and cannot prevent a replaced connection from retiring.

The first commit adds regression coverage for connection ownership, registration races and cleanup, reuse and retirement of busy HTTP/2 connections, checkout deadlines, and abandoned streams. The second commit contains the implementation.

Pooled HTTP/2 connection lifecycle

The pool owns shared connections. Each requester or async consumer owns its streams. A requester exiting cancels its streams without closing the shared connection.

flowchart TD
    A["Request"] --> B["Look up shared HTTP/2 connection<br/>by host, port and TLS options"]
    B -->|Ready| G
    B -->|Missing or busy| C["Acquire per-host connection slot<br/>While waiting, recheck HTTP/2 reuse"]
    C -->|Existing connection becomes ready| G
    C -->|Slot acquired| D["Check out TCP connection<br/>TLS handshake and ALPN"]
    D -->|HTTP/1.1| H["Exclusive request lifecycle<br/>Return or close after response"]
    D -->|HTTP/2| E["Register synchronously with pool"]
    E -->|Existing connection ready| F["Stop duplicate candidate<br/>Use existing connection"]
    F --> G
    E -->|Accept candidate| I["Transfer ownership to pool<br/>Publish for sharing"]
    I --> G["Send request through hackney_conn<br/>h2_connection owns TLS socket"]
    I -->|Replace busy connection| J["Old connection drains tracked streams<br/>Then stops"]
    G --> K["Track each stream and its consumer"]
    K --> L["Receive response<br/>Complete synchronous call or deliver async data"]
    L --> M["Remove stream when consumed"]
    K -->|Consumer exits| N["Cancel and remove its stream"]
    M --> O["Keep shared connection alive<br/>Stop if retiring and empty"]
    N --> O
    E -->|Failure or expiry| P["Stop candidate"]
    J --> Q["Pool observes termination<br/>Removes connection and releases host slot"]
    P --> Q
    O -->|Connection terminates| Q
Loading

@smartinio
smartinio force-pushed the smartinio/fix-http2-pooled-sync-requests branch from 3e26ad7 to ae43641 Compare September 6, 2026 18:29
@smartinio
smartinio marked this pull request as draft September 7, 2026 05:31
@arjan

arjan commented Sep 9, 2026

Copy link
Copy Markdown

This sounds like a really good idea. Have not tried it itself but I had to downgrade my application back from 4.x to 1.x the other day because of the issues described here. Would be awesome if this could be fixed.

@smartinio
smartinio force-pushed the smartinio/fix-http2-pooled-sync-requests branch from ae43641 to 84dc022 Compare September 9, 2026 18:05
@smartinio
smartinio marked this pull request as ready for review September 9, 2026 18:18
@smartinio

Copy link
Copy Markdown
Author

I've ran this PR through several passes of automated code review and am pretty confident it's solid.

@benoitc feel free to discard if you think there is a better solution. Just raising this as a proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants